done

done

There's already a test file multipartUpload.js, wouldn't it be better to add the tests there? Or if a refactor into multiple files is beneficial, maybe port some related tests from there to this new file to avoid scattering the complete MPU tests into multiple files.

moved the tests to multipartUpload.js. This also triggered the new prettier linter in the file, I added it as a separate commit

Shouldn't this be in constants too?

The algorithms object contains the TAG of each algo already, this object is just for testing that the tag was not changed in the algorithms object

Could you construct it from algorithms? If algorithm is supposed to be a constant and we are concerned that it may be accidentally changed, what about using Object.freeze on it?

Is the double space intentional? Also not very fond of matching against such a long error message that may vary, but that's okay I think.

AWS returns two spaces. For all the final API/XML errors I return exactly what AWS returns

 <Error><Code>InvalidPart</Code>
<Message>One or more of the specified parts could not be found.  The part may not have been uploaded, or the specified entity
  tag may not match the part's entity tag.</Message>
<UploadId>CTFNKyL2hI6n6irH0zbVWDzdPZ4n2ueJceRh1juCeuL2X5HOjrvCmXQMEqaoAatEWTEa3pWWxC7t9lOStMzjo0nJb4pv8Ct6oT
  Hv2n8mggVXRQ8RxiXyVyt3.3zpY98HVsZd.ozihhJ1HdUjLCkJtwJMQdNBd4fSdG9drS80vdg-</UploadId>
<PartNumber>1</PartNumber>
<ETag>0ebf9257a12e808d107b2ed1a826c122</ETag>
<RequestId>DAQAPVMCMSY4PDPV</RequestId>
<HostId>XS/2re3ieUQRTKdANLtZv14qyB2h3LjVHnmVrvjP0cj1PazPO16KkArQMtBLBy8S4mmLzQkuXZc=</HostId></Error>

okay fair enough (my little voice tells me that AWS didn't care that much about the beauty of their error messages 🙂 )

I believe no sane application would match an error by their exact error message to the dot, but I'm good with sticking to AWS to avoid ourselves asking the question. Personally I wouldn't care though 😛

Also I believe the error messages are not enforced by the API contract, hence may change at any time on AWS side, which would make our effort useless in the end. Just my two cents and my last comment on this 😉

If the list of supported headers is known and a short list of supported algorithms, it may be cleaner to directly extract each of the possible ones, or otherwise filter like [list-of-supported-headers].includes(h).

It would change the behavior (probably in a good way) if the client sends one or more unsupported checksums along with multiple valid ones, where we probably want to return AlgoNotSupported rather than MultipleChecksumTypes in priority in this case, but that's more a nitpick, either way should work.

AWS checks MultipleChecksumTypes before AlgoNotSupported, the current behavior is the same as AWS.

AWS also ignores only x-amz-checksum-type and x-amz-checksum-algorithm, they don't count them to the checksum count, and they also never trigger AlgoNotSupported. if we send x-amz-checksum-BAD on the other hand we get an AlgoNotSupported. If we send x-amz-checksum-BAD + x-amz-checksum-ZZZ we get MultipleChecksumTypes.

So the order is

1. check no MultipleChecksumTypes
2. check no AlgoNotSupported
3. check actual checksum value, BadDigest if mismatch

I added a commit to ignore x-amz-checksum-type and x-amz-checksum-algorithm in the other functions, I didn't know this behavior existed.

Sounds good to me.

IMHO we don't necessarily have to mimic every AWS behavior when it comes to error handling in particular, as it takes time to reverse engineer those aspects with no clear benefit, and AWS implementation is not perfect and their choices can be debated too. Sure it became the de facto standard and we need to stick to its way of doing things to avoid breaking existing applications, but not necessarily in all its nitty gritty details and quirks.

callNext is not defined anywhere in this file — this will throw a ReferenceError at runtime when computeFinalChecksum returns an error for an explicit (non-default) MPU. Should be next.

— Claude Code

fixed

This diff is too big, it could be in another PR by itself

This pattern reoccurs multiple times in this file, you can make a function for that

You can use assert.rejects

            await assert.rejects(promise, err => {
                assert.strictEqual(
                    err.name,
                    'InvalidPart',
                    `expected InvalidPart, got ${err.name}: ${err.message}`,
                );
               });

debug logs should probably not be tested

I will remove the log tests

In some tests above you use assert.ifError(result) and here assert.strictEqual(result, null), you should stick to only 1 pattern and be consistent everywhere

It would be better to have a full mocked logger like it's done in some other test files, to avoid breakin if we change from debug to another level

I will remove the log tests

Deconstruction could be slightly more readable

Shouldn't this function be another step in the waterfall?

Is there a benefit in catching those errors since we already have a catch-all below

I think a lot of the unit tests could be turned into a declarative matrix for readability.

It seems to me the test code is riddled with this type of local helpers, maybe we should consider a common layer of helpers for MPU construction?

Extra newline and concatenation doesn't look necessary.